-
Notifications
You must be signed in to change notification settings - Fork 18.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add From/To options for Forward and Backward #518
Conversation
Nice! @briancheung you might like to have this for some experiments. I remember Le jeudi 19 juin 2014, longjon notifications@github.com a écrit :
Evan Shelhamer |
Great! This will clean things up a bit, is there documentation for the On Wed, Jun 18, 2014 at 9:57 PM, Evan Shelhamer notifications@github.com
|
@briancheung the code has docstrings, and there are example IPython notebooks; that's the extent of it for now. |
How about a simple test? For instance you could run No strong feelings on this -- if you don't think it's worth it we can just merge. |
It could be worth a little inline documentation to note this is not guaranteed to be correct for DAGs. The sequence of indices between the start and end is not necessarily the path between them since it is up to the order in the model definition. If this sequence is not the path, the computation should be correct but it will compute unnecessary layers off of the path. Another problem case is calling a partial forward from/to/through a fan-in on the DAG; there is no guarantee the other paths will have done their computations so that the bottoms are correct. I'm totally fine with this, since a judicious series of partial forwards gives the correct result, and this is an expert feature. However I'd vote for documenting this. (We could even just write this all up nicely in a comment and put the link in the code.) |
@longjon any thoughts on test or inline documentation? If you think this is fine as is, let's merge. |
@shelhamer, I agree with both your comments. Although I think the code is simple enough that I'm confident in its correctness without a test, future changes might complicate this functionality and make that no longer true. I did have to stop and think about DAGs before doing this, so it's probably worth a comment. I'll probably be able to make those changes by the end of the coming week. I'm fine with either waiting until then or merging this now and doing those things in another PR. |
@shelhamer, I've made the additions you've suggested. Everything passes for me, if Travis agrees I think this is ready for merge. |
Merge it if Travis passes, I am traveling today. Thanks Jon! Le samedi 19 juillet 2014, longjon notifications@github.com a écrit :
|
Add From/To options for Forward and Backward
Add From/To options for Forward and Backward
Add From/To options for Forward and Backward
This PR
Net
for starting or stopping the forward or backward passes at particular layers, andNet.forward
andNet.backward
.This is useful